Skip to content

TPC: adding cluster occupancy to timeseries - #13277

Merged
shahor02 merged 3 commits into
AliceO2Group:devfrom
matthias-kleiner:debug_streamer
Jul 11, 2024
Merged

TPC: adding cluster occupancy to timeseries#13277
shahor02 merged 3 commits into
AliceO2Group:devfrom
matthias-kleiner:debug_streamer

Conversation

@matthias-kleiner

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actions Bot commented Jul 5, 2024

Copy link
Copy Markdown
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass3
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0

@matthias-kleiner

Copy link
Copy Markdown
Contributor Author

Hello @miranov25 @shahor02
I tried to add the occupancy map to the time series, however when I was running it locally with

ARGS_ALL="--shm-segment-size 30000000000"
      o2-global-track-cluster-reader --disable-mc --cluster-types "TOF,TPC" --track-types "ITS,TPC,ITS-TPC,ITS-TPC-TOF,ITS-TPC-TRD-TOF" --primary-vertices --hbfutils-config o2_tfidinfo.root ${ARGS_ALL} \
      | o2-tpc-time-series-workflow --enable-unbinned-root-output --sample-unbinned-tsallis --sampling-factor 0.05  ${ARGS_ALL} -b

I also get negative values. @shahor02 do you know if something is missing in the code?
occupancy

@miranov25

miranov25 commented Jul 5, 2024

Copy link
Copy Markdown
Contributor

The value should always be positive. This appears to be nonsensical—even the value itself seems off. It looks like uninitialized memory junk. I assume that the algorithm should return something, even if it isn’t initialized properly.

@shahor02

shahor02 commented Jul 5, 2024

Copy link
Copy Markdown
Collaborator

Hi @matthias-kleiner

I see that in the GPUParam internally it is interpreted as unsigned int,

const unsigned int* occupancyMap; // Ptr to TPC occupancy map
, please redefine the vector to unsigned int. However, this can hardly explain negative values.
Could you check the size of vector? It should be ~32*3564/8/16.

Pinging to @davidrohr in case he sees something wrong with this way of extracting the occupancy array.

@miranov25

Copy link
Copy Markdown
Contributor

Hello @matthias-kleiner .
Do you have a new version of the tree?

@matthias-kleiner

Copy link
Copy Markdown
Contributor Author

Hi @matthias-kleiner

I see that in the GPUParam internally it is interpreted as unsigned int,

const unsigned int* occupancyMap; // Ptr to TPC occupancy map

, please redefine the vector to unsigned int. However, this can hardly explain negative values.
Could you check the size of vector? It should be ~32*3564/8/16.

Pinging to @davidrohr in case he sees something wrong with this way of extracting the occupancy array.

Thanks, now there are only positive values. The size of the vector is just 3576
occupancy

@shahor02

shahor02 commented Jul 6, 2024

Copy link
Copy Markdown
Collaborator

Thanks, now there are only positive values. The size of the vector is just 3576

@matthias-kleiner , what data are you reading? Is it 128 HBF/orbit?

@miranov25

Copy link
Copy Markdown
Contributor

Hello @matthias-kleiner and @shahor02

Based on our test with Jens, I would expect to see approximately 900 values for standard settings. As Ruben suggested, it appears that you might be using the 128 HBF/orbit configuration. Are you using the pp 500 kHz data in that test?

If it is pp 500 kHz, then the occupancy aligns closely with my expectations.

Could you display the occupancy versus time? I suspect the smaller values are at the beginning and the end od the slot . I’m unsure about the spike at the end.

@miranov25

Copy link
Copy Markdown
Contributor

The best will be to give me the access to that particular tree

@miranov25

Copy link
Copy Markdown
Contributor

Here's an improved version of your message for better clarity and formality:


I attempted to open the file provided by Matthias Kleiner, but encountered some issues with reading it, which seems to be a separate problem.
Inu data:
(lustre/nyx/alice/users/mkleiner/test_prod/testtsPP/o2_timeseries_tpc.root. It is 2024 pp data LHC24af/550367

However, I'm unclear about what happens when you request clusters using the following code:

dataRequest->requestClusters(GID::getSourcesMask("TPC"), useMC);

We are utilizing the workflow offline , and I'm not certain if we will obtain an occupancy estimator in this context. I had assumed that occupancy is calculated during the tracking phase while reading clusters.

Is this approach feasible?

@matthias-kleiner

Copy link
Copy Markdown
Contributor Author

I was running again, but this time with the full reconstruction with the O2DPG scripts instead of loading the tracks and clusters from the local files. Now It seems more reasonable. The size is also now just 912.
occupancy

@miranov25

Copy link
Copy Markdown
Contributor

Hello @matthias-kleiner

Thank you. Good. Is this information included in the new file you provided? Still the spike at 0 is somehow surprising, so I have to check the content - after recompilation of O2.

However, this seems to be bad news. It appears that to obtain occupancy data, we must run the full reconstruction, and we cannot repeat the time series extraction without completing the entire reconstruction process.

@miranov25

miranov25 commented Jul 6, 2024

Copy link
Copy Markdown
Contributor

The value from your plot, approximately 4.2 x 10^9, seems excessively high. This is likely due to a non-initialized array. It appears we should wait for David's input, as that number seems nonsensical.

For reference:

root [25] TMath::Power(2,32)
(double) 4.2949673e+09

@miranov25

Copy link
Copy Markdown
Contributor

While I'm waiting for the O2 to recompile to the latest version, could you create a histogram for only a subset of the data? Here's the command:

tree->Draw("mOccupancyMapTPC>>his(100,0,2000000)", "", "")

I'm trying to determine whether all the data are nonsensical or if only part of it is undefined.

@shahor02

shahor02 commented Jul 6, 2024

Copy link
Copy Markdown
Collaborator

We are utilizing the workflow offline , and I'm not certain if we will obtain an occupancy estimator in this context. I had assumed that occupancy is calculated during the tracking phase while reading clusters.

If you are using o2-global-track-cluster-reader for the input when running over existing reconstruction, you just need to add --cluster-types TPC to its options.

@shahor02

shahor02 commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

@matthias-kleiner @miranov25 note that the 1st bin of the array gives the integrated Nclusters, the timebins start from offset of 2:

if (occupancymap) {
mParam->occupancyTotal = *occupancymap;
if (mParam->rec.tpc.occupancyMapTimeBins) {
mParam->occupancyMap = occupancymap + 2;
}
}

@shahor02

shahor02 commented Jul 10, 2024

Copy link
Copy Markdown
Collaborator

@matthias-kleiner do you have the log of the test with reader? As far as I can see the code, the GRP is loaded and for me the log says at 1st TF:

[3022078:tpc-clusters-sharing-map-producer]: [18:45:55][INFO] Processing timeslice:0, tfCounter:183583, firstTForbit:33800384, runNumber:544508, creation:1697087333586, action:0
[3022078:tpc-clusters-sharing-map-producer]: [18:45:55][INFO] Caching in 128416549254992 ptr to GLO/GRPECS/0 (0x37dd540)
[3022078:tpc-clusters-sharing-map-producer]: [18:45:55][INFO] Will use 32 HB per TF from GRPECS
[3022078:tpc-clusters-sharing-map-producer]: [18:45:56][INFO] Timing for TPC clusters sharing map creation: Cpu: 6.500e-01 Real: 6.578e-01 s
[3022078:tpc-clusters-sharing-map-producer]: [18:45:56][INFO] Done processing timeslice:0, tfCounter:183583, firstTForbit:33800384, runNumber:544508, creation:1697087333586, action:0, wall:676816439

@miranov25

Copy link
Copy Markdown
Contributor

Hello @matthias-kleiner, @shahor02, @davidrohr,

We need the code for the apass4 production starting next week. From what we understand, the content of the histogram is reasonable, but the size has not been correctly initialized. I propose temporarily hardwiring the histogram length if this issue isn't resolved today.

This approach was already agreed upon in our discussion with Jens. Do you also agree? If so, @matthias-kleiner, could you implement this change?

We want to make similar inclusion into the AO2D, I was discussing that notion with David (Chinelatto) in mattermost.

Best regards,

Marian

@matthias-kleiner

Copy link
Copy Markdown
Contributor Author

Hello @matthias-kleiner, @shahor02, @davidrohr,

We need the code for the apass4 production starting next week. From what we understand, the content of the histogram is reasonable, but the size has not been correctly initialized. I propose temporarily hardwiring the histogram length if this issue isn't resolved today.

This approach was already agreed upon in our discussion with Jens. Do you also agree? If so, @matthias-kleiner, could you implement this change?

We want to make similar inclusion into the AO2D, I was discussing that notion with David (Chinelatto) in mattermost.

Best regards,

Marian

As I understand for the apass4 production the size will be correct, as I observed the problem with the size only when re-running the workflow with the local files. I think we can fix this also later?

@davidrohr

davidrohr commented Jul 11, 2024 via email

Copy link
Copy Markdown
Collaborator

@miranov25

Copy link
Copy Markdown
Contributor

@matthias-kleiner , can you make fix as @davidrohr suggested?

@matthias-kleiner

Copy link
Copy Markdown
Contributor Author

@matthias-kleiner , can you make fix as @davidrohr suggested?

I can restrict it to a maximum of 912 and make it configutable. Or is there some way to calculate the size what it should be for given number of orbits per TF?

@matthias-kleiner
matthias-kleiner marked this pull request as ready for review July 11, 2024 16:52
@miranov25

Copy link
Copy Markdown
Contributor

I suggest to merge and add it to the dependencies for PBpb apass4. Can you post new file to the lustre so I can check the content?

@shahor02
shahor02 merged commit fba0d4f into AliceO2Group:dev Jul 11, 2024
@shahor02 shahor02 added the async-2023-pbpb-apass4 Request porting to async-2023-pbpb-apass4 label Jul 11, 2024
chiarazampolli pushed a commit that referenced this pull request Jul 12, 2024
* TPC: adding cluster occupancy to timeseries

* fixing type of vector

* Restricting maximum size of occpancy vector

(cherry picked from commit fba0d4f)
@chiarazampolli chiarazampolli removed the async-2023-pbpb-apass4 Request porting to async-2023-pbpb-apass4 label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants